feat: add CI mode to sql-tap client for automated N+1 and slow query detection by mickamy · Pull Request #26 · mickamy/sql-tap

mickamy · 2026-03-11T01:18:17Z

No description provided.

…detection

Copilot

Pull request overview

Adds a non-interactive “CI mode” to the sql-tap client so it can run in automated environments, collect query events until termination, then output a summary report and exit with a failure code when N+1/slow queries are detected.

Changes:

Add --ci flag to switch between TUI monitoring and CI collection/reporting.
Introduce new ci package to stream events from the TapService, aggregate N+1/slow-query problems, and render a report.
Add unit tests for CI aggregation and report formatting.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
main.go	Adds `--ci` flag and SIGINT/SIGTERM-driven CI execution path.
ci/ci.go	Implements CI streaming, aggregation, and report formatting for detected problems.
ci/ci_test.go	Adds unit tests covering aggregation, reporting, and edge cases.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-11T01:22:24Z

+	// gRPC wraps context errors in status; unwrap and check.
+	if s, ok := status.FromError(err); ok {
+		msg := s.Message()
+		return strings.Contains(msg, "context canceled") ||
+			strings.Contains(msg, "context deadline exceeded")
+	}
+	return false


isStreamDone determines cancellation/deadline by substring-matching status.FromError(err).Message(). This is brittle (message text can change, be wrapped, or be localized) and can cause CI runs to fail with an error when they should exit cleanly on SIGINT/SIGTERM. Prefer checking status.Code(err) for codes.Canceled / codes.DeadlineExceeded (and/or errors.Is(err, context.Canceled)), and consider updating the server-side Watch handler to return those gRPC status codes on context cancellation so the client can reliably detect stream termination.

Copilot · 2026-03-11T01:22:24Z

+	if nq := e.GetNormalizedQuery(); nq != "" {
+		return nq
+	}
+	return e.GetQuery()


normalizedOrRaw falls back to emitting the raw SQL when NormalizedQuery is empty. Raw queries may include literal values (PII/secrets) and this CI report is likely to be stored in build logs. Elsewhere (e.g., TUI analytics/export) the code avoids using raw queries when normalization is missing. Consider normalizing locally (e.g., via the existing query.Normalize) or redacting/omitting the query text when a normalized form isn't available, to avoid leaking sensitive data in CI output.

Suggested change

return e.GetQuery()

// Avoid emitting raw SQL, which may contain PII/secrets, in CI output.

return "<query omitted>"

Copilot · 2026-03-11T01:22:24Z

+func collect(ctx context.Context, stream tapv1.TapService_WatchClient) (Result, error) {
+	var events []*tapv1.QueryEvent
+
+	for {
+		resp, err := stream.Recv()
+		if err != nil {
+			if isStreamDone(ctx, err) {
+				break
+			}
+			return Result{}, fmt.Errorf("recv: %w", err)
+		}
+		events = append(events, resp.GetEvent())
+	}
+
+	return Aggregate(events), nil
+}


collect appends every received event into an in-memory slice and only aggregates at the end. In CI mode this runs until SIGINT/SIGTERM, so memory usage can grow without bound and potentially OOM on long/verbose runs. Consider aggregating incrementally while streaming (update grouped stats per event) and only keep the aggregates needed for the final report, rather than storing all events.

…memory

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-11T01:32:25Z

+}
+
+// Run connects to the gRPC server at addr, collects query events until ctx is
+// cancelled, and returns the aggregated result.


The Run doc comment says it "collects query events until ctx is cancelled", but collect also stops on io.EOF (server-side stream completion). Please adjust the comment so it matches the behavior (cancellation or stream end).

Suggested change

// cancelled, and returns the aggregated result.

// cancelled or the server closes the stream, and returns the aggregated result.

Copilot · 2026-03-11T01:32:25Z

+			fmt.Fprintf(&b, "  [N+1]  %s  (detected %d times)\n", p.Query, p.Count)
+		case ProblemSlowQuery:
+			avg := p.AvgDuration.Truncate(time.Millisecond)
+			fmt.Fprintf(&b, "  [SLOW] %s  (avg %s, %d occurrences)\n", p.Query, avg, p.Count)


Result.Report() silently ignores Problem.Kind values other than N+1/SLOW (no default case), which can produce a "Problems found" section with missing entries since Problem/ProblemKind are exported and can be constructed externally. Consider adding a default case (or an explicit unknown formatter) so reports stay informative even if new/invalid kinds appear.

Suggested change

fmt.Fprintf(&b, " [SLOW] %s (avg %s, %d occurrences)\n", p.Query, avg, p.Count)

fmt.Fprintf(&b, " [SLOW] %s (avg %s, %d occurrences)\n", p.Query, avg, p.Count)

default:

fmt.Fprintf(&b, " [%s] %s (%d occurrences)\n", string(p.Kind), p.Query, p.Count)

Copilot · 2026-03-11T01:32:25Z

+// Run connects to the gRPC server at addr, collects query events until ctx is
+// cancelled, and returns the aggregated result.
+func Run(ctx context.Context, addr string) (Result, error) {
+	conn, err := grpc.NewClient(addr, grpc.WithTransportCredentials(insecure.NewCredentials()))
+	if err != nil {
+		return Result{}, fmt.Errorf("dial %s: %w", addr, err)
+	}
+	defer func() { _ = conn.Close() }()
+
+	client := tapv1.NewTapServiceClient(conn)
+	stream, err := client.Watch(ctx, &tapv1.WatchRequest{})
+	if err != nil {
+		return Result{}, fmt.Errorf("watch %s: %w", addr, err)
+	}
+
+	return collect(ctx, stream)
+}


The streaming path (Run/collect/isStreamDone) isn’t covered by tests in this package. Since CI mode relies on correct cancellation/EOF handling and error classification, consider adding an integration-style test that starts an in-process gRPC TapService (similar to server/server_test.go) and asserts that ci.Run returns aggregated results and exits cleanly on context cancellation.

Copilot · 2026-03-11T01:32:25Z

+		return 1
+	}
+
+	fmt.Fprint(os.Stderr, result.Report())


-ci mode prints the report to stderr. In CI environments this can be treated as an error stream even when the run succeeds, and it also makes it harder to pipe/report the output. Consider printing the report to stdout and reserving stderr for actual errors.

Suggested change

fmt.Fprint(os.Stderr, result.Report())

fmt.Fprint(os.Stdout, result.Report())

Copilot · 2026-03-11T01:32:26Z

 	}

 	showVersion := fs.Bool("version", false, "show version and exit")
+	ciMode := fs.Bool("ci", false, "run in CI mode: collect events until SIGTERM/SIGINT, then report and exit")


The -ci flag help text says it runs "until SIGTERM/SIGINT", but the implementation will also exit if the gRPC stream ends (e.g., server stops or connection drops). Please update the help string to reflect the actual termination conditions to avoid confusing CI users.

Suggested change

ciMode := fs.Bool("ci", false, "run in CI mode: collect events until SIGTERM/SIGINT, then report and exit")

ciMode := fs.Bool("ci", false, "run in CI mode: collect events until SIGTERM/SIGINT or the stream ends, then report and exit")

- Update doc comment and help text to mention stream-end termination - Print CI report to stdout instead of stderr - Add default case in Report for unknown ProblemKind

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

feat: add CI mode to sql-tap client for automated N+1 and slow query …

8ddbfd9

…detection

Copilot AI review requested due to automatic review settings March 11, 2026 01:18

Copilot started reviewing on behalf of mickamy March 11, 2026 01:18 View session

Copilot AI reviewed Mar 11, 2026

View reviewed changes

mickamy added 3 commits March 11, 2026 10:23

lint: remove unused nolint directives in proxy tests

6096baa

fix: use gRPC status codes instead of string matching in isStreamDone

af8e09b

refactor: use streaming aggregation instead of storing all events in …

30c2562

…memory

Copilot AI review requested due to automatic review settings March 11, 2026 01:28

Copilot started reviewing on behalf of mickamy March 11, 2026 01:28 View session

Copilot AI reviewed Mar 11, 2026

View reviewed changes

mickamy added 2 commits March 11, 2026 10:34

fix: address review comments on CI mode

fd83936

- Update doc comment and help text to mention stream-end termination - Print CI report to stdout instead of stderr - Add default case in Report for unknown ProblemKind

test: add integration tests for ci.Run with in-process gRPC server

4384ae3

mickamy requested a review from Copilot March 11, 2026 06:40

Copilot started reviewing on behalf of mickamy March 11, 2026 06:41 View session

Copilot AI reviewed Mar 11, 2026

View reviewed changes

mickamy merged commit 272ec8c into main Mar 11, 2026
5 checks passed

mickamy deleted the feat/ci branch March 11, 2026 06:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: add CI mode to sql-tap client for automated N+1 and slow query detection#26

feat: add CI mode to sql-tap client for automated N+1 and slow query detection#26
mickamy merged 6 commits intomainfrom
feat/ci

mickamy commented Mar 11, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 11, 2026

Uh oh!

Copilot AI Mar 11, 2026

Uh oh!

Copilot AI Mar 11, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 11, 2026

Uh oh!

Copilot AI Mar 11, 2026

Uh oh!

Copilot AI Mar 11, 2026

Uh oh!

Copilot AI Mar 11, 2026

Uh oh!

Copilot AI Mar 11, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	return e.GetQuery()
	// Avoid emitting raw SQL, which may contain PII/secrets, in CI output.
	return "<query omitted>"

	// cancelled, and returns the aggregated result.
	// cancelled or the server closes the stream, and returns the aggregated result.

	fmt.Fprint(os.Stderr, result.Report())
	fmt.Fprint(os.Stdout, result.Report())

	ciMode := fs.Bool("ci", false, "run in CI mode: collect events until SIGTERM/SIGINT, then report and exit")
	ciMode := fs.Bool("ci", false, "run in CI mode: collect events until SIGTERM/SIGINT or the stream ends, then report and exit")

Uh oh!

Conversation

mickamy commented Mar 11, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants